fix(log_lib): run_with_log hangs forever when an orphan holds the child's pipe - #7
Open
tigist-far wants to merge 1 commit into
Open
tigist-far wants to merge 1 commit into
tigist-far wants to merge 1 commit into
Conversation
…ld's pipe `process_subprocess_stream` reads the child's output until EOF, and only then does `run_with_log` call `proc.wait()` -- with the comment "Stream processing already waited for process completion". That assumption breaks whenever a grandchild outlives the child: EOF needs EVERY write end of the pipe closed, including the ones the grandchild inherited, so the reader stays blocked, `proc.wait()` is never reached, and `run_with_log` never returns a returncode. The caller hangs with no timeout. Seen in production on a two-node training job: the ranks aborted on an RDMA transport error, torchrun exited, but five orphaned multiprocessing-spawn children kept the stdout pipe open. The Ray task running the command never finished, so the managed-jobs controller reported the job as RUNNING for two hours while 16 GPUs sat idle. `kill_children_processes` cannot help: it walks the process tree, and the orphans are reparented to init, so they are no longer in it. The process group still reaches them -- `start_new_session=True` makes the child a group leader, so the group id is its pid, and neither reparenting nor reaping the leader moves the orphans out of that group. A watchdog thread now waits for the child, then waits a grace period for the readers to drain, and only if they are still blocked kills the leftover process group to force EOF. That trigger matters: an unconditional kill after exit would break commands that deliberately leave daemons in the group, such as `ray start`. When it does fire, the caller was already hung forever, so forcing EOF cannot lose anything that was still working. Test: a child that exits while a background grandchild holds its stdout. Without the fix it hangs (killed at 90 s); with it, `run_with_log` returns the child's exit code after the grace period. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
levfarai
approved these changes
Sep 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
run_with_logreads the child's output until EOF and only then callsproc.wait(), on the assumption stated in the code that "Stream processing already waited for process completion". EOF needs every write end of the pipe closed, including the ones a grandchild inherited, so whenever a grandchild outlives the child the reader blocks forever,proc.wait()is never reached, and the function never returns a return code. There is no timeout on that path.Fix. A watchdog thread waits for the child, gives the readers a grace period (30 s,
_ORPHANED_PIPE_GRACE_SECONDS) to drain, and only if they are still blocked after it kills the child's process group (os.killpg(pgid, SIGKILL)) to force EOF.sequenceDiagram participant C as child (start_new_session) participant G as orphaned grandchildren participant R as run_with_log readers participant W as watchdog thread C->>G: spawn (inherit stdout/stderr pipe) C-->>W: exits (rc known) Note over G: reparented to init, pipe still open Note over R: blocked in read: no EOF W->>W: wait grace period for the readers to drain W->>G: killpg(child pid, SIGKILL) Note over R: EOF -> proc.wait() -> return rcWhy it matters
Observed in production on a two-node training job. The ranks aborted on an RDMA transport error and torchrun exited, but five orphaned
multiprocessing.spawnchildren kept the stdout pipe open:The Ray task running the command never finished, so the managed-jobs controller reported
JobStatus.RUNNINGfor two hours (#RECOVERIES 0) while 16 GPUs sat idle. Autorecovery cannot fire on a job that never reports failure.Design points
kill_children_processeswalks the process tree, and orphans are reparented to init, so they have left it.start_new_session=Truemakes the child a session and group leader, so its group id is its pid, and neither reparenting nor reaping the leader moves the orphans out of the groupproc.pid, notos.getpgid(proc.pid)getpgidon it can fail; a first attempt did that and degraded silently to a no-opray startamong them). When the narrow condition holds the caller was already hung forever, so forcing EOF cannot lose anything still workingAreas changed
sky/skylet/log_lib.py_force_eof_when_orphans_hold_the_pipewatchdog thread started byrun_with_logafter the child is spawned;_ORPHANED_PIPE_GRACE_SECONDS = 30; the readers signal athreading.Eventwhen drainedtests/unit_tests/test_sky/skylet/test_log_lib.pyrun_with_logreturns the child's exit code after the grace period; without it the test hangs (killed at 90 s)Testing
tests/unit_tests/test_sky/skylet/test_log_lib.py: 13 passed in 4.9 s (the new test included).yapf(pinned 0.32.0),isortand the whitespace hooks are clean. The fork has no GitHub Actions on pull requests.